Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui: Always show main navigation Key/Value link #10916

Merged
merged 3 commits into from
Sep 22, 2021

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Aug 26, 2021

Currently there is no way for us to use our HTTP authorization API
endpoint to tell us whether a user has access to any KVs (including the
case where a user may not have access to the root KV store, but do have
access to a sub item)

This is a little weird still as in the above case the user would click
on this link and still get a 403 for the root, and then have to manually
type in the URL for the KV they do have access to.

Despite this we think this change makes sense as at least something about KV is
visible in the main navigation.

Once we have the ability to know if any KVs are accessible, we can add
this guard back in.

We'd initially just removed the logic around the button, but then
noticed there may be further related KV issues due to the nested nature
of KVs so we finally decided on simply ignoring the responses from the
HTTP API, essentially reverting the KV area back to being a thin client.
This means when things are revisted in the backend we can undo this
change easily in one place.

@johncowen johncowen added theme/ui Anything related to the UI backport/1.10 labels Aug 26, 2021
@johncowen johncowen requested a review from kaxcode August 26, 2021 11:35
Currently there is no way for us to use our HTTP authorization API
endpoint to tell us whether a user has access to any KVs (including the
case where a user may not have access to the root KV store, but do have
access to a sub item)

This is a little weird still as in the above case the user would click
on this link and still get a 403 for the root, and then have to manually
type in the URL for the KV they do have access to.

Despite this we think this change makes sense as at least something about KV is
visible in the main navigation.

Once we have the ability to know if any KVs are accessible, we can add
this guard back in.

We'd initially just removed the logic around the button, but then
noticed there may be further related KV issues due to the nexted nature
of KVs so we finally decided on simply ignoring the responses from the
HTTP API, essentially reverting the KV area back to being a thin client.
This means when things are revisted in the backend we can undo this
easily change in one place.
Copy link
Contributor

@kaxcode kaxcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@kaxcode
Copy link
Contributor

kaxcode commented Aug 31, 2021

Test needs to be update to see kvs on navigation.

@hashicorp-cla
Copy link

hashicorp-cla commented Sep 9, 2021

CLA assistant check
All committers have signed the CLA.

@vercel vercel bot temporarily deployed to Preview – consul September 22, 2021 17:02 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging September 22, 2021 17:02 Inactive
@johncowen johncowen merged commit cf638ee into main Sep 22, 2021
@johncowen johncowen deleted the ui/bugfix/enable-kv-main-nav branch September 22, 2021 17:24
@hc-github-team-consul-core
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/453533.

@hc-github-team-consul-core
Copy link
Contributor

🍒✅ Cherry pick of commit cf638ee onto release/1.10.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Sep 22, 2021
* ui: Ignore response from API for KV permissions

Currently there is no way for us to use our HTTP authorization API
endpoint to tell us whether a user has access to any KVs (including the
case where a user may not have access to the root KV store, but do have
access to a sub item)

This is a little weird still as in the above case the user would click
on this link and still get a 403 for the root, and then have to manually
type in the URL for the KV they do have access to.

Despite this we think this change makes sense as at least something about KV is
visible in the main navigation.

Once we have the ability to know if any KVs are accessible, we can add
this guard back in.

We'd initially just removed the logic around the button, but then
noticed there may be further related KV issues due to the nested nature
of KVs so we finally decided on simply ignoring the responses from the
HTTP API, essentially reverting the KV area back to being a thin client.
This means when things are revisited in the backend we can undo this
easily change in one place.

* Move acceptance tests to use ACLs perms instead of KV ones
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
4 participants